Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.77%. Comparing base (bc728d3) to head (985b955).
Report is 1 commits behind head on main.

@@           Coverage Diff           @@
##             main     #634   +/-   ##
=======================================
  Coverage   78.77%   78.77%           
=======================================
  Files           9        9           
  Lines        3849     3849           
=======================================
  Hits         3032     3032           
  Misses        817      817           

Context:

@mcbarton said:

Once I take this in #634 , I highly advised that no PR be merged into CppInterOp, without squashing the commits into one. Failure to do so, will likely cause all other work across the Github organisation to grind to a complete halt. This is because each commit will need all workflow runs to complete on main. To complete all jobs for one commit takes quite a while for CppInterOp, so multiple commits will use up all available Github runners for several hours. Also if the commits are dependent on each other to build CppInterOp, or pass the workflow, then the workflow runs for specific commits could fail on main, wasting Github minutes (although we effectively have infinite minutes for Github runners as an open source project, eventually they start throttling your usage). I will merge it tomorrow to give people time to read this message.

@Vipul-Cariappa said:

Is this temporary?
If yes, can we fix the PR to not run the CI for each commit, then merge it?
If not, WE SHOULD NOT MERGE THIS.

I mostly use squash and merge, but there are valid situations where we want to present the commit history.

Why do we want this? I don't know the pros of this PR. But the cons are severe.

@Vipul-Cariappa You will need to make your case to @vgvassilev if you do not have this in. He opened up it up as a bug (see #597). I provided a fix, but gave a warning about the consequences.

It is not possible to merge all commits of a PR, but only run the workflow for the most recent one. Currently if you merge 2 PR into main, before the workflow has finished running for the first one, then the workflow run will get cancelled for the first PR. That is all I will say on the matter.

Ok, I understand the purpose.
I will let @vgvassilev take the call.

Points to note: (let me know if I am wrong)

• We can still perform a rebase and merge, but it will end up running the CI for all the commits.
• We always merge after the CI is green. CI merges the source branch against main before running the tests. If there are merge conflicts, CI is not run until they are resolved. So, do we still want this change?

The way I understand this feature is that it allows us to cancel the current pr build when a new push to the same pr was done but it won’t cancel builds after something was merged. I did something similar in clad.

It probably needs to be done like this:

https://github.com/vgvassilev/clad/blob/c15f37a873b1e21b341fb164d804782dc7996e84/.github/workflows/ci.yml#L13

@vgvassilev The way the feature works, is that if two workflows share the same group, when cancel-in-progress is set to true, then it will cancel the older workflow run.

The bug that this PR fixes exists in clad too. e.g.
The workflow run for this commit that was merged into main https://github.com/vgvassilev/clad/actions/runs/15684475337/job/44184036519 was cancelled because the commit was this workflow run was merged in https://github.com/vgvassilev/clad/actions/runs/15684717247/job/44184903750 .

@vgvassilev The way the feature works, is that if two workflows share the same group, when cancel-in-progress is set to true, then it will cancel the older workflow run.

The bug that this PR fixes exists in clad too. e.g. The workflow run for this commit that was merged into main https://github.com/vgvassilev/clad/actions/runs/15684475337/job/44184036519 was cancelled because the commit was this workflow run was merged in https://github.com/vgvassilev/clad/actions/runs/15684717247/job/44184903750 .

What’s the fix for this. How does that work in root?

Looking at root, they don't seem to have the bug, and the workflow run is only for the latest commit of PR when its merged in. This is what they have defined for the group https://github.com/root-project/root/blob/2a6eaa17d8cdb82ea548d1c45deedf2759260b90/.github/workflows/root-ci.yml#L77 . Can't say I understand how that fixes the bug, but we can try it out. Before I make this change to the PR, I will take a closer look at roots ci, to check nothing else is at play to make this happen.

@vgvassilev I changed this to roots solution to the problem. Can't say I understand how it works. I have also updated xeus-cpps PR to this solution here compiler-research/xeus-cpp#352 . If you can give your approval again to both PRs, then I will take both in.

@Vipul-Cariappa whats your take?

If we can rebase and merge without bottlenecking the CI, I am good.

@mcbarton, I assume rebase and merge will now only run the CI for the latest commit and not all the commits. And it also solves Vassil's issue of cancelling the CI for old merges when there is a new merge.

If we can rebase and merge without bottlenecking the CI, I am good.

@mcbarton, I assume rebase and merge will now only run the CI for the latest commit and not all the commits. And it also solves Vassil's issue of cancelling the CI for old merges when there is a new merge.

@Vipul-Cariappa I believe it solves both problems looking at roots ci running, but don't know for certain as I don't understand how roots solution is achieving this result. Best I can say is we can try it, and if it doesn't work as intended, then we can revert the change. Let me know if your happy with this.

If we can rebase and merge without bottlenecking the CI, I am good.

@mcbarton, I assume rebase and merge will now only run the CI for the latest commit and not all the commits. And it also solves Vassil's issue of cancelling the CI for old merges when there is a new merge.

It is the opposite. When we merge to main we don’t want to cancel on subsequent merges but if a push to a pr branch happens we should cancel all builds.

If we can rebase and merge without bottlenecking the CI, I am good.
@mcbarton, I assume rebase and merge will now only run the CI for the latest commit and not all the commits. And it also solves Vassil's issue of cancelling the CI for old merges when there is a new merge.

It is the opposite. When we merge to main we don’t want to cancel on subsequent merges but if a push to a pr branch happens we should cancel all builds.

This definitely happens on roots main (see image below)

Going by this PR in root https://github.com/root-project/root/pull/19144/commits and searching through the commit workflow runs, then it appears to cancel past workflow runs if a new commit is made to a PR. Therefore I think it solves both problems, but still not 100% sure.

I guess we can merge this. Revert/update if this is not what we want.
From my understanding of @mcbarton, I am fine with this. 👍🏽